Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@yjbanov
Copy link
Contributor

@yjbanov yjbanov commented Dec 13, 2024

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

@github-actions github-actions bot added the platform-web Code specifically for the web engine label Dec 13, 2024
@jtmcdole
Copy link
Member

This and some ET problems?

00:00 �[32m+0�[0m�[31m -1�[0m: test smoke test -- this test SHOULD FAIL �[1m�[31m[E]�[0m�[0m

  Expected: true
    Actual: <false>


  Expected: true
    Actual: <false>
Invalid argument (name): Expected to start with a valid platform name (i.e. linux/) or "ci/": "web_build/artifacts"
#0      mangleConfigName (package:engine_tool/src/build_utils.dart:57:3)

@eyebrowsoffire
Copy link
Contributor

I'm confused by this change. Does this mean we are going to actually build all the artifacts twice in every presubmit?

@yjbanov
Copy link
Contributor Author

yjbanov commented Dec 13, 2024

@eyebrowsoffire

I'm confused by this change. Does this mean we are going to actually build all the artifacts twice in every presubmit?

Unfortunately for now, yes. The good news is that it shouldn't be too much overhead. Currently, linux_web_engine performs the following work:

  • Build:
    • The engine build itself - 13 min (but due to drone overhead it currently costs 20 min).
    • Engine tests - 8 x 7 min = 56 min
  • Test:
    • Chrome: 2 x 25 min = 50 min
    • Safari: 12 min
    • Firefox: 12 min

So that's 20 + 56 + 50 + 12 + 12 = 150 min worth of work total.

The new linux_web_engine_build builder duplicates just the 13 min (we don't need drones, so there's no extra 7 min overhead), and it moves the artifact upload work to itself so there's no duplication there.

So overall, I do not expect more than 15% overhead total. However, it will remove massive amounts of overhead from the merge queue by reducing the amount of work from 150 min to something <20min.

Copy link
Member

@jtmcdole jtmcdole left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we think we can safely land this before the repo; then lets get a win.

@yjbanov
Copy link
Contributor Author

yjbanov commented Dec 18, 2024

Superseded by #57232

@yjbanov yjbanov closed this Dec 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

platform-web Code specifically for the web engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[monorepo] need build-only builder for the web engine

3 participants